Reviewing files that changed from the base of the PR and between 33c98f1 and 887b314.

• examples/dynamic_path/main.go (1 hunks)
• server/sse.go (9 hunks)
• server/sse_test.go (1 hunks)

Reviewing files that changed from the base of the PR and between 887b314 and 15e1a5e.

• examples/dynamic_path/main.go (1 hunks)
• server/sse.go (9 hunks)
• server/sse_test.go (2 hunks)

• examples/dynamic_path/main.go
• server/sse.go

server/sse_test.go (4)

865-957: Well-structured test for dynamic path mounting

This test case thoroughly verifies the new dynamic base path functionality, correctly testing both the SSE endpoint connection and JSON-RPC message communication with tenant-specific paths.

Note that the test uses Go 1.22+ PathValue() function, which should be mentioned in documentation as a dependency requirement.

---

958-980: Important safety check for dynamic path configuration

Good test ensuring that the API enforces correct usage patterns - SSEHandler and MessageHandler work correctly in both modes, while ServeHTTP properly panics when configured with dynamic paths to prevent misuse.

---

1022-1057: Comprehensive URL generation test for all configuration combinations

Great test case that verifies the URL generation logic for all combinations of static vs dynamic paths, with and without base URL, and with full URL usage enabled or disabled. This ensures consistent behavior across all configuration patterns.

---

1059-1070: Important API contract enforcement

Good test verifying that CompleteSseEndpoint correctly panics when used with dynamic base paths, enforcing the API contract and preventing misuse.

Reviewing files that changed from the base of the PR and between 15e1a5e and 252d7d8.

• examples/dynamic_path/main.go (1 hunks)
• server/sse.go (9 hunks)
• server/sse_test.go (3 hunks)

• examples/dynamic_path/main.go
• server/sse_test.go

server/sse.go (9)

36-41: Well-defined type for dynamic base path handling

The new DynamicBasePathFunc type is clearly documented and serves an important purpose in enabling multi-tenant and dynamic routing scenarios.

---

79-80: LGTM - Clear field documentation

The field comment clearly explains the purpose of this function pointer for determining dynamic base paths.

---

120-136: Good implementation with path sanitization

The implementation of WithDynamicBasePath is robust with proper path sanitization that ensures consistent path formats (adding leading slash when missing and removing trailing slashes). This prevents URL construction issues.

---

359-374: Clean implementation of dynamic path support in endpoint generation

The updated GetMessageEndpointForClient method properly handles both static and dynamic base paths with a clear implementation. The method intelligently applies the dynamic base path function when available.

---

485-488: Good defensive programming with clear error guidance

The panic with a clear error message helps prevent misuse of the API by enforcing the correct usage pattern when using dynamic base paths.

---

501-503: Consistent API enforcement pattern

Consistent with the implementation in CompleteSseEndpoint, ensuring that these methods can't be misused with dynamic base paths.

---

515-571: Excellent handler abstractions with comprehensive documentation

The new SSEHandler and MessageHandler methods are well-designed and thoroughly documented with example usage. They provide clean abstractions for integrating with various HTTP routers and supporting dynamic path segments.

---

575-577: Strong API consistency enforcement

The panic in ServeHTTP properly prevents API misuse and clearly guides users toward the correct approach for dynamic path scenarios.

---

339-340: LGTM - Updated handleSSE to support dynamic paths

Properly updated to pass the request to GetMessageEndpointForClient, enabling dynamic base path resolution.

Reviewing files that changed from the base of the PR and between 252d7d8 and a643f22.

• examples/dynamic_path/main.go (1 hunks)
• server/sse.go (9 hunks)
• server/sse_test.go (3 hunks)

• examples/dynamic_path/main.go
• server/sse_test.go

server/sse.go (8)

36-41: Well-documented type for dynamic base path generation

Good definition with clear documentation explaining the purpose and expected behavior of the function type.

---

79-81: Clean field addition with descriptive comment

The new field for storing the dynamic base path function is well-commented and properly placed in the struct.

---

120-136: Excellent path sanitization implementation

The implementation correctly normalizes paths by ensuring they start with a slash and don't end with one, preventing potential URL construction issues. This is consistent with the learning from a previous review about preventing double slashes.

---

359-374: Clean implementation of dynamic endpoint generation

The method correctly handles both static and dynamic paths, with proper fallback and consistent URL construction. The implementation is straightforward and maintains backward compatibility.

---

485-489: Good enforcement of usage patterns

Appropriate panic messages when attempting to use static methods with dynamic paths, guiding users toward the correct pattern. This helps prevent subtle bugs that could arise from mixing the two approaches.

Also applies to: 501-504

---

515-542: Well-documented handler for SSE endpoint

The SSEHandler method has excellent documentation with clear examples that show how to use it with dynamic paths. The examples are particularly helpful for showing the integration with routers.

---

544-571: Well-documented handler for message endpoint

Similar to the SSE handler, the documentation is thorough with relevant examples. The consistent approach between both handlers makes the API intuitive to use.

---

575-577: Clear enforcement of the correct usage pattern

The panic message explicitly directs users to the appropriate methods for dynamic path scenarios, preventing misuse of the API.

Reviewing files that changed from the base of the PR and between 86cebe8 and 4abd640.

• examples/dynamic_path/main.go (1 hunks)
• server/sse.go (9 hunks)
• server/sse_test.go (3 hunks)

• examples/dynamic_path/main.go
• server/sse_test.go

server/sse.go (10)

36-41: Well-documented new type DynamicBasePathFunc

This new type definition is well-documented with a clear description of its purpose and expected usage. The function signature is appropriately designed to accept both the HTTP request and session ID as parameters, providing the necessary context for computing dynamic paths.

---

79-80: Well-placed field addition for dynamic base path function

The SSEServer struct is appropriately extended with the dynamicBasePathFunc field to store the user-provided function. The comment accurately describes its purpose.

---

120-136: Good implementation of path sanitization in WithDynamicBasePath

The implementation correctly wraps the user-provided function to ensure path normalization (adding leading slash if missing and removing trailing slash). This aligns with the retrieved learning and prevents URL construction issues.

---

339-340: Correct update to handleSSE for dynamic base path support

The code now correctly passes the HTTP request to GetMessageEndpointForClient, which is necessary for dynamic base path resolution in the initial endpoint event.

---

362-374: Robust implementation of GetMessageEndpointForClient

The method now properly handles both static and dynamic base paths. The logic is clear and correctly applies the configured options, including the full URL usage flag.

---

486-488: Clear boundary enforcement in CompleteSseEndpoint

The panic message clearly indicates that this method shouldn't be used with dynamic base paths and suggests the correct approach.

However, based on a previous review comment, there was a discussion about avoiding panics in favor of standard Go error patterns.

Consider using an error return instead of a panic, as discussed in previous feedback:

-func (s *SSEServer) CompleteSseEndpoint() string {
+func (s *SSEServer) CompleteSseEndpoint() (string, error) {
 	if s.dynamicBasePathFunc != nil {
-		panic("CompleteSseEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.")
+		return "", fmt.Errorf("CompleteSseEndpoint cannot be used with WithDynamicBasePath; use dynamic path logic in your router")
 	}
-	return s.baseURL + s.basePath + s.sseEndpoint
+	return s.baseURL + s.basePath + s.sseEndpoint, nil
}

---

501-503: Clear boundary enforcement in CompleteMessageEndpoint

Similar to CompleteSseEndpoint, this method uses a panic to clearly indicate that it's incompatible with dynamic base paths.

The same concern applies here regarding the use of panics.

Consider using an error return instead of a panic, as discussed in previous feedback:

-func (s *SSEServer) CompleteMessageEndpoint() string {
+func (s *SSEServer) CompleteMessageEndpoint() (string, error) {
 	if s.dynamicBasePathFunc != nil {
-		panic("CompleteMessageEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.")
+		return "", fmt.Errorf("CompleteMessageEndpoint cannot be used with WithDynamicBasePath; use dynamic path logic in your router")
 	}
-	return s.baseURL + s.basePath + s.messageEndpoint
+	return s.baseURL + s.basePath + s.messageEndpoint, nil
}

---

515-543: Well-documented SSEHandler method with clear usage examples

This new method provides a clear, clean interface for mounting the SSE handler at any path. The documentation is excellent, with helpful examples and important usage notes. The implementation is simple and delegates to the existing handler function.

---

544-571: Well-documented MessageHandler method with clear usage examples

Like the SSEHandler method, this provides a clean interface for mounting the message handler. The documentation is clear and includes helpful examples that match those in the SSEHandler documentation for consistency.

---

575-577: Clear boundary enforcement in ServeHTTP

The method correctly prevents the use of ServeHTTP with dynamic base paths, directing users to the appropriate alternative methods.

However, similar to the other methods, the use of panic here could be reconsidered.

Consider handling this case with an error response instead of a panic:

func (s *SSEServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	if s.dynamicBasePathFunc != nil {
-		panic("ServeHTTP cannot be used with WithDynamicBasePath. Use SSEHandler/MessageHandler and mount them with your router.")
+		http.Error(w, "ServeHTTP cannot be used with WithDynamicBasePath. Use SSEHandler/MessageHandler and mount them with your router.", http.StatusInternalServerError)
+		return
	}
	// Rest of the method...
}

Reviewing files that changed from the base of the PR and between 304fe1a and ebb4e67.

• examples/dynamic_path/main.go (1 hunks)
• server/errors.go (2 hunks)
• server/sse.go (7 hunks)
• server/sse_test.go (3 hunks)

• server/errors.go
• examples/dynamic_path/main.go
• server/sse_test.go

server/sse.go (8)

36-41: Well-designed interface for dynamic base path generation

The DynamicBasePathFunc type definition provides a clear and flexible interface for generating base paths dynamically. The documentation clearly explains the purpose and expected return values.

---

79-81: Good field documentation for the dynamic base path function

The comment clearly indicates the purpose of this field in the SSEServer struct.

---

120-136: Excellent path sanitization implementation

The path sanitization logic ensures base paths always start with a leading slash and have no trailing slash, preventing URL construction issues. This implementation matches the learnings from previous PRs and properly handles nil functions.

---

485-490: Good error handling for incompatible dynamic configuration

Returning a specialized error when attempting to use static path methods with dynamic configuration is a clean approach that provides clear guidance to users.

---

504-509: Consistent error handling pattern

The error handling approach is consistent with CompleteSseEndpoint, providing a unified experience across the API.

---

523-550: Comprehensive documentation with practical examples

The documentation for SSEHandler is excellent - it clearly explains the purpose, usage scenarios, and provides concrete examples. The emphasis on using WithDynamicBasePath with these handlers is important for correctness.

---

552-579: Consistent handler implementation and documentation

The MessageHandler implementation follows the same pattern as SSEHandler, with equally thorough documentation.

---

583-586: Appropriate enforcement of usage patterns

Returning an HTTP 500 error with a clear message when attempting to use ServeHTTP with dynamic configuration is a good way to enforce correct usage patterns and guide users to the appropriate methods.